fix(resource/mongodbatlas_advanced_cluster): Emit warning when use_effective_fields and auto-scaling are enabled and spec fields change#4405
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the advanced cluster plan modification logic to warn users when use_effective_fields = true is combined with auto-scaling and the configuration changes spec fields that Atlas will ignore.
Changes:
- Emit a Terraform warning when
use_effective_fields=true, auto-scaling is enabled, andinstance_size/disk_size_gb/disk_iopschange. - Refactor
autoScalingUsedto accept a single cluster model and adjust auto-scaling keep-unknown detection accordingly.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| autoScalingFields := determineKeepUnknownsAutoScaling(ctx, diags, state, plan) | ||
| keepUnknown = append(keepUnknown, autoScalingFields...) | ||
| emitWarningIfSpecChangedWithAutoScaling(ctx, diags, plan, attributeChanges) | ||
| schemafunc.CopyUnknowns(ctx, state, plan, keepUnknown, nil) |
There was a problem hiding this comment.
This PR introduces new user-facing behavior (a warning when use_effective_fields=true, auto-scaling is enabled, and spec fields change), but there’s no automated coverage asserting when the warning should/shouldn’t be emitted. Adding a focused unit test around emitWarningIfSpecChangedWithAutoScaling (or exercising it via an existing plan modifier test harness) would help prevent regressions and false positives.
There was a problem hiding this comment.
There's not a way of asserting warnings into the test framework. The code path is exercised by the existing TestAccAdvancedCluster_effectiveComputeAutoScalingInstanceSize test, which runs the exact scenario. A panic or logic error would be caught there.
Behaviour of the warning is documented and manual tests are done in the PR description.
There was a problem hiding this comment.
as discussed offline, consider if it's worth it to add some unit tests
There was a problem hiding this comment.
Added in c43d6c8.
Also added a comment on why the duplication of schema attributes to avoid uppercasing in schema.go, feel free to review and if too verbose, I can revert it
|
APIx bot: a message has been sent to Docs Slack channel |
🤖 Augment PR SummarySummary: This PR adds a plan-time warning for Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
| // emitWarningIfSpecChangedWithAutoScaling warns when use_effective_fields=true and auto-scaling is enabled but the user | ||
| // changed instance_size, disk_size_gb, or disk_iops. Atlas silently ignores these changes in that combination | ||
| func emitWarningIfSpecChangedWithAutoScaling(ctx context.Context, diags *diag.Diagnostics, plan *TFModel, attributeChanges schemafunc.AttributeChanges) { | ||
| if !plan.UseEffectiveFields.ValueBool() || !autoScalingUsed(ctx, diags, plan) { |
There was a problem hiding this comment.
q: autoScalingUsed ORs auto_scaling and analytics_auto_scaling, but per Atlas behavior auto_scaling.compute_enabled only governs electable/read_only specs and analytics_auto_scaling.compute_enabled only governs analytics specs (disk is governed by the regular block alone). so auto_scaling.compute_enabled = false + analytics_auto_scaling.compute_enabled = true + change to electable_specs.instance_size -> warning fires, but Atlas honors the change
is this v1 over-approximation acceptable (worth a follow-up ticket), or worth scoping the check per-block before merge?
There was a problem hiding this comment.
I'd go with the approach of trying to scope this check before the merge so I can bring it to the tech sync and align with the team. Working on investigating the possibility of this
| diags.AddWarning( | ||
| "Spec change ignored due to auto-scaling", | ||
| fmt.Sprintf("The change to %s will be stored in Terraform state but will not modify the actual cluster in Atlas. "+ | ||
| "When use_effective_fields is true and auto-scaling is enabled, Atlas controls instance_size, disk_size_gb, and disk_iops values. "+ | ||
| "To apply this change, temporarily disable auto-scaling. "+ | ||
| "See: https://registry.terraform.io/providers/mongodb/mongodbatlas/latest/docs/resources/advanced_cluster#manually-updating-specs-with-use_effective_fields", | ||
| strings.Join(changedFields, ", ")), | ||
| ) |
There was a problem hiding this comment.
nit: style drifts a bit from the existing diagnostics in this file (e.g. the read_only_specs AddError at lines 37-42 — specific title naming the trigger, second-person voice, full action sequence). consider matching, e.g.
| diags.AddWarning( | |
| "Spec change ignored due to auto-scaling", | |
| fmt.Sprintf("The change to %s will be stored in Terraform state but will not modify the actual cluster in Atlas. "+ | |
| "When use_effective_fields is true and auto-scaling is enabled, Atlas controls instance_size, disk_size_gb, and disk_iops values. "+ | |
| "To apply this change, temporarily disable auto-scaling. "+ | |
| "See: https://registry.terraform.io/providers/mongodb/mongodbatlas/latest/docs/resources/advanced_cluster#manually-updating-specs-with-use_effective_fields", | |
| strings.Join(changedFields, ", ")), | |
| ) | |
| diags.AddWarning( | |
| "Spec change ignored when use_effective_fields is true and auto-scaling is enabled", | |
| fmt.Sprintf("Your changes to %s will be stored in Terraform state but will not modify the actual cluster in Atlas. "+ | |
| "When use_effective_fields is true and auto-scaling is enabled, Atlas controls instance_size, disk_size_gb, and disk_iops values. "+ | |
| "To apply your changes, disable auto-scaling and apply, then re-enable auto-scaling in a separate apply. "+ | |
| "See: https://registry.terraform.io/providers/mongodb/mongodbatlas/latest/docs/resources/advanced_cluster#manually-updating-specs-with-use_effective_fields", | |
| strings.Join(changedFields, ", ")), | |
| ) |
separately: not sure about having the link as it might get outdated. also please check the message looks fine when multiple fields are changed simultaneously.
There was a problem hiding this comment.
Addressed in 80a7e52.
Note that the link is added per the acceptance criteria of the ticket:
Acceptance Criteria
Warning includes the specific field(s) being changed and a link to the documentation
If we remove the link, we'd also have to adjust the acceptance criteria
There was a problem hiding this comment.
i see the risk of the link getting outdated but ok to leave
| autoScalingFields := determineKeepUnknownsAutoScaling(ctx, diags, state, plan) | ||
| keepUnknown = append(keepUnknown, autoScalingFields...) | ||
| emitWarningIfSpecChangedWithAutoScaling(ctx, diags, plan, attributeChanges) | ||
| schemafunc.CopyUnknowns(ctx, state, plan, keepUnknown, nil) |
There was a problem hiding this comment.
as discussed offline, consider if it's worth it to add some unit tests
| } | ||
| } | ||
|
|
||
| func buildPlanWithAutoScaling(t *testing.T, useEffectiveFields, computeEnabled, diskGBEnabled bool) *advancedcluster.TFModel { |
There was a problem hiding this comment.
consider changing WarnIgnoredSpecChange signature to take useEffectiveFields and autoScalingEnabled bool instead of ctx and plan *TFModel, moving the autoScalingUsed call to the call site in handleModifyPlan.
this way unit tests pass booleans directly with no framework objects to construct
Description
Emits a Terraform warning during
ModifyPlanwhen all three conditions are met:use_effective_fields = truecompute_enabled = trueordisk_gb_enabled = true)instance_size,disk_size_gb, ordisk_iopsIn this combination, the Atlas API silently ignores the spec change, the new values are stored in Terraform state but the cluster is never modified, producing a false "1 changed" result with no feedback. The warning makes this visible and directs the user to temporarily disable auto-scaling to apply the change.
Link to any related issue(s): https://jira.mongodb.org/browse/CLOUDP-388940
Type of change:
Required Checklist:
Testing
The warning itself cannot be asserted programmatically. The Terraform Testing framework has no mechanism to assert warning. The code path is exercised by the existing
TestAccAdvancedCluster_effectiveComputeAutoScalingInstanceSize, where if there are any code regressions the issue would be caught. Manual validation has been done in order to ensure the warning surfaces according to the acceptance criteria:use_effective_fields= truecompute_enabled = trueordisk_gb_enabled = true)instance_size,disk_size_gb, ordisk_iopsBaseline config:
instance_size = "M10"and auto-scaling enabledinstance_sizeM10 → M20 (auto-scaling on)The change to instance_size will be stored in Terraform state but will not modify the actual cluster in Atlas.disk_size_gb10 → 20 (auto-scaling on)The change to disk_size_gb will be stored in Terraform state but will not modify the actual cluster in Atlas.disk_iops3000 → 4000 (auto-scaling on)The change to disk_iops will be stored in Terraform state but will not modify the actual cluster in Atlas.instance_size(auto-scaling off)use_effective_fieldsand changeinstance_sizeM10 → M20Full warning message (case 2)
Further comments